Add WireGuard support for tunnel traffic encryption #2297

xliuxu · 2021-06-23T01:51:37Z

Add WireGuard client for tunnel traffic encryption. This PR implements #2243.

codecov-commenter · 2021-06-23T02:50:05Z

Codecov Report

Merging #2297 (b054878) into main (f425a87) will increase coverage by 5.31%.
The diff coverage is 70.14%.

@@            Coverage Diff             @@
##             main    #2297      +/-   ##
==========================================
+ Coverage   60.34%   65.66%   +5.31%     
==========================================
  Files         283      285       +2     
  Lines       22866    26335    +3469     
==========================================
+ Hits        13798    17292    +3494     
+ Misses       7585     7427     -158     
- Partials     1483     1616     +133

Flag	Coverage Δ
e2e-tests	`57.15% <58.76%> (?)`
kind-e2e-tests	`48.35% <55.67%> (+0.29%)`	⬆️
unit-tests	`41.07% <38.06%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/config/traffic_encap_mode.go	`94.73% <ø> (-5.27%)`	⬇️
...gent/controller/noderoute/node_route_controller.go	`62.23% <65.78%> (+7.02%)`	⬆️
pkg/agent/wireguard/client_linux.go	`66.96% <66.96%> (ø)`
pkg/agent/agent.go	`59.09% <72.72%> (+8.67%)`	⬆️
pkg/agent/openflow/client.go	`71.71% <72.72%> (+14.11%)`	⬆️
pkg/agent/route/route_linux.go	`48.76% <77.77%> (+5.76%)`	⬆️
pkg/agent/config/node_config.go	`94.11% <83.33%> (-5.89%)`	⬇️
pkg/agent/config/traffic_encryption_mode.go	`91.66% <91.66%> (ø)`
pkg/agent/openflow/pipeline.go	`83.62% <100.00%> (+10.41%)`	⬆️
pkg/controller/egress/ipallocator/allocator.go	`65.00% <0.00%> (-15.42%)`	⬇️
... and 278 more

xliuxu · 2021-06-28T15:25:28Z

/test-e2e

xliuxu · 2021-06-28T23:44:44Z

/test-e2e

xliuxu · 2021-06-30T00:41:33Z

/test-e2e

xliuxu · 2021-06-30T07:15:50Z

/test-all

xliuxu · 2021-06-30T11:32:40Z

/test-conformance
/test-e2e

xliuxu · 2021-07-01T01:03:15Z

/test-e2e

jianjuns

LGTM

antoninbas

sorry for the late review

antoninbas · 2021-08-26T18:09:31Z

build/yamls/antrea-aks.yml

+    #trafficEncryptionMode: none
+
+    # The port for WireGuard to receive traffic.
+    #wireGuardPort: 51820


have we decided that it is unlikely we will be adding more wireguard config parameters in the future? I'm referring to an earlier suggestion of grouping wireguard parameters in their own dictionary.

I just saw your reply: #2297 (comment)
refactoring in the future is not great as it breaks backward compatibility. I'll leave it up to you to decide based on how confident you are that we will not need additional configuration parameters in the future.

I changed it to group WireGuard related settings together. PTAL.

cmd/antrea-agent/agent.go

hack/generate-manifest.sh

pkg/agent/openflow/client_test.go

pkg/agent/wireguard/client_linux.go

pkg/agent/types/annotations.go

antoninbas · 2021-08-26T18:42:53Z

build/yamls/base/conf/antrea-agent.conf

@@ -77,8 +77,18 @@ featureGates:
 # also adjust MTU to accommodate for tunnel encapsulation overhead (if applicable).
 #defaultMTU: 0

-# Whether or not to enable IPsec encryption of tunnel traffic.
-#enableIPSecTunnel: false
+# Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.


Trying to understand "Currently encryption only works with antrea encap mode."
I see a check in the code for IPsec but not for Wireguard. I was also under the assumption that with @tnqn's design, the encap mode didn't really matter since we would always end up using a Wireguard tunnel. It seems to me that we are running the WireGuard e2e tests on Kind in all traffic modes, including noEncap.

Good catch. In this implementation, WG is like a new encap mode. But it wont work with networkPolicyOnly right? I need to think through the relations between encap mode and WG..

(Originally I suggested to support only encap mode the same way as IPsec for a shared implementation, but as the current code does not looks complex either, I am not against to it).

Probably we should still require trafficEncryptionMode = encap, and add a comment on tunnelType saying tunnelType will not take effect when trafficEncryptionMode = wireGuard.

Maybe also move trafficEncryptionMode to be after tunnelType.

Thanks for pointing it out. WG is like a new encap mode. I have updated the comments.

Originally, I thought WG should be viewed as encap, as it has the same behavior with other tunnel types from user perspective, and it is definitely not noEncap or hybrid (which require underlay network to forward/route Pod traffic). So, in my mind, we should enforce trafficEncapMode = encap, and document tunnelType will not take effect. I talked to @xliuxu about this offline.

However, after more thinking, I got the question on networkPolicyOnly mode. Originally I thought WG cannot work with networkPolicyOnly mode, but now I start to think it depends on how we define networkPolicyOnly mode: if we expect another CNI to handle connectivity/routing, then when WG is enabled, what that CNI does might not take effect; but if we assume another CNI could just do IPAM and users do not care how forwarding/routing is done, then WG might still work with the CNI.

But at least for this version, I still suggest to enforce trafficEncapMode = encap, and we can spend more time to think through the use case of another IPAM + WG, and validate it indeed works (let me know if we already tested WG + networkPolicyOnly on AKS and EKS).

@tnqn @antoninbas : thoughts.

maybe I'm over simplifying but it feels like Wireguard is really just an encrypted overlay. In a way, wireguard could really be merged with tunnelType and tunnelType could have the following values: vxlan, geneve, gre, vxlanIPsec, geneveIPsec, greIPsec, wireguard. It feels to me that for for networkPolicyOnly mode (IPAM + WG), it's not really specific to Wireguard at all. You could do the same thing with a Geneve overlay. At the end of the day, packets are encapsulated and I don't think that what you expect of networkPolicyOnly mode. Am I missing something?

For this PR / release, I agree with you: we should enforce trafficEncapMode == encap.

You can also say that. But major code changes are needed to support another IPAM with Geneve (while it is much easier with WG, as WG handles traffic routing among Nodes).

Thought over. Probably we should keep networkPolicyOnly as is (it means another CNI should take care of traffic routing across Nodes and it could support traffic encryption too). Later, if there are requirements, we can support other IPAM plugin separately (with Antrea handles routing).

You can also say that. But major code changes are needed to support another IPAM with Geneve (while it is much easier with WG, as WG handles traffic routing among Nodes).

It's pretty similar no? With Wireguard, you still have to install Pod routes through the wireguard device on the host and configure the wireguard peers, while with Geneve it would be OVS flows. So the implementation is a bit different, but the amount of information you need is the same (list of Pods IPs for each Node)?

jianjuns · 2021-08-26T19:03:54Z

pkg/agent/openflow/client.go

+		// tunnelPeerIP is the Node Internal Address. In a dual-stack setup, one Node has 2 Node Internal
+		// Addresses (IPv4 and IPv6) .
+		if c.networkConfig.TrafficEncryptionMode != config.TrafficEncryptionModeWireGuard &&
+			((!isIPv6 && c.networkConfig.TrafficEncapMode.NeedsEncapToPeer(tunnelPeerIPs.IPv4, c.nodeConfig.NodeTransportIPv4Addr)) ||


Probably we can consider to change NeedsEncapToPeer to a func of NetworkConfig, and cover TrafficEncryptionModeWireGuard. No strong opinion.

I second that. I forgot to leave a comment in my review but I was going to suggest moving the checks to a dedicated function or updating NeedsEncapToPeer.

If we decide WG requires trafficEncapMode = encap, probably we still keep the current definition (or we rename NeedsEncapToPeer to sth. like NeedsOVSTunnelToPeer and move it to NetworkConfig and cover TrafficEncryptionModeWireGuard).

I think WG can work with all trafficEncapModes. I moved the function NeedsEncapToPeer to NetworkConfig to cover TrafficEncryptionMode settings.

jianjuns · 2021-08-26T19:16:32Z

build/yamls/base/conf/antrea-agent.conf

@@ -77,8 +77,18 @@ featureGates:
 # also adjust MTU to accommodate for tunnel encapsulation overhead (if applicable).
 #defaultMTU: 0

-# Whether or not to enable IPsec encryption of tunnel traffic.
-#enableIPSecTunnel: false
+# Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.


Good catch. In this implementation, WG is like a new encap mode. But it wont work with networkPolicyOnly right? I need to think through the relations between encap mode and WG..

(Originally I suggested to support only encap mode the same way as IPsec for a shared implementation, but as the current code does not looks complex either, I am not against to it).

xliuxu · 2021-08-27T00:06:47Z

Thanks @antoninbas and @jianjuns for the review. I have addressed all comments. PTAL.

xliuxu · 2021-08-27T00:16:32Z

/test-all

build/yamls/base/conf/antrea-agent.conf

cmd/antrea-agent/config.go

tnqn · 2021-08-27T02:33:53Z

cmd/antrea-agent/options.go

@@ -134,8 +138,8 @@ func (o *Options) validate(args []string) error {
 		if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) {
 			return fmt.Errorf("TrafficEncapMode %s requires AntreaProxy to be enabled", o.config.TrafficEncapMode)
 		}
-		if o.config.EnableIPSecTunnel {
-			return fmt.Errorf("IPsec tunnel may only be enabled in %s mode", config.TrafficEncapModeEncap)
+		if encryptionMode == config.TrafficEncryptionModeIPSec {


From the discussion, I think this should check encryptionMode != TrafficEncryptionModeNone now?

tnqn · 2021-08-27T02:37:03Z

pkg/agent/config/node_config.go

+// NeedsDirectRoutingToPeer returns true if Pod traffic to peer Node needs a direct route installed to the routing table.
+func (nc *NetworkConfig) NeedsDirectRoutingToPeer(peerIP net.IP, localIP *net.IPNet) bool {
+	if nc.TrafficEncryptionMode == TrafficEncryptionModeWireGuard {
+		return true


This change seems not required and might be incorrect? We install direct route when NeedsDirectRoutingToPeer is true. I see you check TrafficEncryptionMode == TrafficEncryptionModeWireGuard first before checking this, then this change is not needed anway.

I deleted the check for WireGuard in this function.

jianjuns · 2021-08-27T03:39:05Z

build/yamls/antrea-aks.yml

    # - geneve (default)
    # - vxlan
    # - gre
    # - stt
    #tunnelType: geneve

+    # Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.


Nit: antrea encap mode -> encap mode.

Thanks. Updated.

jianjuns · 2021-08-27T03:42:05Z

build/yamls/base/conf/antrea-agent.conf

@@ -77,8 +77,18 @@ featureGates:
 # also adjust MTU to accommodate for tunnel encapsulation overhead (if applicable).
 #defaultMTU: 0

-# Whether or not to enable IPsec encryption of tunnel traffic.
-#enableIPSecTunnel: false
+# Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.


You can also say that. But major code changes are needed to support another IPAM with Geneve (while it is much easier with WG, as WG handles traffic routing among Nodes).

Thought over. Probably we should keep networkPolicyOnly as is (it means another CNI should take care of traffic routing across Nodes and it could support traffic encryption too). Later, if there are requirements, we can support other IPAM plugin separately (with Antrea handles routing).

jianjuns · 2021-08-27T03:53:02Z

pkg/agent/config/node_config.go

@@ -126,3 +142,17 @@ func IsIPv6Enabled(nodeConfig *NodeConfig, trafficEncapMode TrafficEncapModeType
 	return nodeConfig.PodIPv6CIDR != nil ||
 		(trafficEncapMode.IsNetworkPolicyOnly() && nodeConfig.NodeIPv6Addr != nil)
 }
+
+// NeedsEncapToPeer returns true if Pod traffic to peer Node needs to be encapsulated.


"needs to be encapsulated by OVS tunneling"

Actually, I prefer to rename the func to NeedsTunnelToPeer, and here we can say "needs to be tunneled".

ok. updated.

antoninbas

Looks good on my side

This PR implements antrea-io#2243. Change tunnel traffic encryption option to enum type. The options contains none (default), ipsec and wireguard. Signed-off-by: Xu Liu <[email protected]>

tnqn

LGTM

tnqn · 2021-08-27T05:36:22Z

/test-all

tnqn · 2021-08-27T06:27:40Z

/test-conformance
/test-ipv6-only-e2e
/test-ipv6-ds-e2e

tnqn · 2021-08-27T07:20:25Z

/test-ipv6-e2e

xliuxu changed the title ~~Add WireGuard client for tunnel traffic encryption. This PR is a part~~ Add WireGuard client for tunnel traffic encryption. Jun 23, 2021

xliuxu changed the title ~~Add WireGuard client for tunnel traffic encryption.~~ Add WireGuard client for tunnel traffic encryption Jun 23, 2021

xliuxu force-pushed the wireguard_client branch 4 times, most recently from b823ec3 to bd9fa04 Compare June 23, 2021 02:34

xliuxu force-pushed the wireguard_client branch from bd9fa04 to cd127f5 Compare June 23, 2021 03:26

xliuxu marked this pull request as draft June 24, 2021 03:02

xliuxu force-pushed the wireguard_client branch 7 times, most recently from 2a6e9cf to 2564d69 Compare June 28, 2021 14:59

xliuxu marked this pull request as ready for review June 28, 2021 15:25

xliuxu force-pushed the wireguard_client branch from 2564d69 to a14ad1d Compare June 29, 2021 15:02

xliuxu force-pushed the wireguard_client branch from a14ad1d to 5a751f2 Compare June 30, 2021 06:38

xliuxu changed the title ~~Add WireGuard client for tunnel traffic encryption~~ Add WireGuard support for tunnel traffic encryption Jun 30, 2021

xliuxu force-pushed the wireguard_client branch from 5a751f2 to aac4e7a Compare June 30, 2021 06:56

xliuxu force-pushed the wireguard_client branch 2 times, most recently from 5166d25 to 8ef795f Compare June 30, 2021 08:04

xliuxu force-pushed the wireguard_client branch from 8ef795f to ff855dd Compare July 2, 2021 02:29

jianjuns reviewed Aug 26, 2021

View reviewed changes

antoninbas reviewed Aug 26, 2021

View reviewed changes

jianjuns reviewed Aug 26, 2021

View reviewed changes

xliuxu dismissed tnqn’s stale review via 4d6e025 August 26, 2021 22:59

xliuxu force-pushed the wireguard_client branch 3 times, most recently from 92f942e to 54859f4 Compare August 26, 2021 23:27

antoninbas reviewed Aug 27, 2021

View reviewed changes

build/yamls/base/conf/antrea-agent.conf Outdated Show resolved Hide resolved

build/yamls/base/conf/antrea-agent.conf Outdated Show resolved Hide resolved

tnqn reviewed Aug 27, 2021

View reviewed changes

cmd/antrea-agent/config.go Outdated Show resolved Hide resolved

tnqn reviewed Aug 27, 2021

View reviewed changes

xliuxu force-pushed the wireguard_client branch 2 times, most recently from fbb0bf5 to 859f43b Compare August 27, 2021 03:24

jianjuns previously approved these changes Aug 27, 2021

View reviewed changes

xliuxu dismissed jianjuns’s stale review via 871cc44 August 27, 2021 04:10

xliuxu force-pushed the wireguard_client branch from 859f43b to 871cc44 Compare August 27, 2021 04:10

antoninbas previously approved these changes Aug 27, 2021

View reviewed changes

Add WireGuard support for tunnel traffic encryption

b054878

This PR implements antrea-io#2243. Change tunnel traffic encryption option to enum type. The options contains none (default), ipsec and wireguard. Signed-off-by: Xu Liu <[email protected]>

xliuxu dismissed antoninbas’s stale review via b054878 August 27, 2021 04:30

xliuxu force-pushed the wireguard_client branch from 871cc44 to b054878 Compare August 27, 2021 04:30

jianjuns approved these changes Aug 27, 2021

View reviewed changes

tnqn approved these changes Aug 27, 2021

View reviewed changes

tnqn merged commit a2aac03 into antrea-io:main Aug 27, 2021

tnqn mentioned this pull request Aug 27, 2021

WireGuard on Antrea implementation #2243

Closed

4 tasks

xliuxu deleted the wireguard_client branch August 27, 2021 13:15

Add WireGuard support for tunnel traffic encryption #2297

Add WireGuard support for tunnel traffic encryption #2297

Conversation

xliuxu commented Jun 23, 2021 • edited Loading

codecov-commenter commented Jun 23, 2021 • edited Loading

Codecov Report

xliuxu commented Jun 28, 2021

xliuxu commented Jun 28, 2021

xliuxu commented Jun 30, 2021

xliuxu commented Jun 30, 2021

xliuxu commented Jun 30, 2021

xliuxu commented Jul 1, 2021

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xliuxu commented Aug 27, 2021

xliuxu commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 27, 2021

tnqn commented Aug 27, 2021

tnqn commented Aug 27, 2021

xliuxu commented Jun 23, 2021 •

edited

Loading

codecov-commenter commented Jun 23, 2021 •

edited

Loading